-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for members with accessors in class_ #130
base: master
Are you sure you want to change the base?
Conversation
The tests above fail on old versions of V8. (I'm testing against 7.8.279.23) |
Thank you very much for the contribution! I will review and merge it. I would prefer to stay with the elder V8 version, as possible. Currently Travis CI runs tests for different V8 versions starting from 6.3, in development I use 7.5. It seems that V8 has some changes in its API, so the movement to the newer V8 is inevitable. In this case a minimal V8 version should be a reasonable value, e.g. the same as used in Node.js LTS version. Best regards |
Added ifdefs for the V8 version. NewFromUtf8 that returns a Local (And not MaybeLocal) was deprecated in 7.6 |
Hi @yonatan-mitmit, thanks for the pointing out the issue with I've fixed this in context of issue #131, it works with V8 version 7.9. I need to add the newer version to Travis build matrix. |
Thanks the fix. Thanks! |
Sorry for the review delay, I had no spare time. The proposed changes look good 👍 Could you please also add a test case, in order to demonstrate how such a property would be bind in C++ and used in JavaScript? Best regards, |
14bc459
to
352c061
Compare
bd106c3
to
6611145
Compare
4119f8e
to
211ef9b
Compare
The PR contains the following changes:
P.S.
Thanks for the awesome library